Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed autoincrement id fetching cache problem in import/export module #3730

Merged
merged 20 commits into from
Jan 23, 2024

Conversation

leissbua
Copy link
Contributor

@leissbua leissbua commented Jan 11, 2024

Description (*)

Import/Export fetches an autoincrement id via getNextAutoincrement($tableName) by mysql SHOW TABLE STATUS.
That table information is cached in standard mySQL and should be cached. In first place I would say, that handling of the autoincrement register of a table should only be managed by mysql, but here we are, dealing with Magento.
The change is simply to set information_schema_stats_expiry to 0 before fetching the autoincrement. This will only be valid for the connection session and does not create a performance problem for the framework in other places, where caching is absolutely wanted. In general. import/export module is the only caller to that function, so to disable the information schema cache in general would not be a good idea.

Manual testing scenarios (*)

Make sure information_schema_stats_expiry is set default 86400 seconds in mysql8. Import a configurable product in replace mode, having at least one child. Go to e.g. table catalog_product_super_attribute and check the autoincremented product_super_attribute_id.
Do nothing that could trigger a mysql cache refresh. Do the same import again, and you will end up having exactly the same product_super_attribute_id from the first import.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added the Component: ImportExport Relates to Mage_ImportExport label Jan 11, 2024
@leissbua
Copy link
Contributor Author

This is a serious bug, that gave us headaches when using import/export. The bug is real, and can easily be reproduced, by doing two small imports (single row) consecutively. Feedback would be appreciated.

@fballiano
Copy link
Contributor

@colinmollenhour what do you think about this?

@colinmollenhour
Copy link
Member

Lots of previous discussion here: #1721
Wow, I never saw any info that indicated that SHOW TABLE STATUS uses the information_schema stats cache as well so that is quite surprising but it is true, I verified it myself as well.

I think this PR should be accepted as well as #1721, but the SET statement should only be called once per session so a static variable should be added so it can be skipped on subsequent calls.

@colinmollenhour
Copy link
Member

As an aside, does anyone know of a situation where turning off the information schema stats cache has a real impact on performance? Maybe it should just be recommended to disable it globally as the side effects are not well-known or documented..

@kiatng
Copy link
Contributor

kiatng commented Jan 19, 2024

I am not sure, but information_schema_stats_expiry was introduced in MySQL 8.0.3:

image

Would this cause problem with MySQL 5.7?

@kiatng
Copy link
Contributor

kiatng commented Jan 19, 2024

For ref, alternate solution from https://github.com/mash2/cobby-magento/blob/master/src/app/code/community/Mash2/Cobby/Helper/Resource.php

    /**
     * Find next value of autoincrement key for specified table.
     *
     * @param string $tableName
     * @throws Exception
     * @return string
     */
    public function getNextAutoincrement($tableName, $primaryKey)
    {
        $connection = Mage::getSingleton('core/resource')->getConnection('read');
        $select = $connection->select();
        $select->from($tableName, 'MAX('.$primaryKey.')');
        $result = (int)$connection->fetchOne($select) +1;
        return $result;
    }

The cobby module was original referenced in issue #1712, which it seems no longer use showTableStatus().

@leissbua
Copy link
Contributor Author

@colinmollenhour I added a semaphore to only set schema stats expiry to 0 once. I am happy to receive some feeedback.

@colinmollenhour
Copy link
Member

I am not sure, but information_schema_stats_expiry was introduced in MySQL 8.0.3
Would this cause problem with MySQL 5.7?

Yes, I think it would.. Great observation! Seems this should be wrapped in an if clause based on server version...

@fballiano
Copy link
Contributor

I am not sure, but information_schema_stats_expiry was introduced in MySQL 8.0.3
Would this cause problem with MySQL 5.7?

Yes, I think it would.. Great observation! Seems this should be wrapped in an if clause based on server version...

but also what about mariadb or other mysql derivatives?

@sreichel
Copy link
Contributor

but also what about mariadb or other mysql derivatives?

https://dba.stackexchange.com/questions/323392/missing-information-schema-stats-expiry-in-mariadb-server-system-variables

@colinmollenhour
Copy link
Member

The simple solution would be to just wrap it in a try/catch and ignore any errors. 😁

@leissbua
Copy link
Contributor Author

I added the try catch as @colinmollenhour proposed and followed @fballiano hint and removed the unneeed comment

README.md Outdated Show resolved Hide resolved
@leissbua
Copy link
Contributor Author

I added all your suggestions into the fix, I would be interested in your opinions or other suggestions. From my point of view, this will solve a massive problem with the import/export module that damages configurable/simple relations in catalog_product_super_attribute table.

Copy link
Member

@colinmollenhour colinmollenhour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks!

Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
fballiano
fballiano previously approved these changes Jan 23, 2024
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine by me

Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine by me

@fballiano fballiano merged commit f34dbe3 into OpenMage:main Jan 23, 2024
17 checks passed
@fballiano fballiano changed the title Fix autoincrement id fetching cache problem in import/export module Fixed autoincrement id fetching cache problem in import/export module Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: ImportExport Relates to Mage_ImportExport documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants